Conversation
Janther
force-pushed
the
constructor-experiment
branch
4 times, most recently
from
bc3b4af to
bc1f2cf
Compare
Janther
force-pushed
the
constructor-experiment
branch
3 times, most recently
from
d9a6fa9 to
3440d44
Compare
Janther
force-pushed
the
constructor-experiment
branch
from
1db4664 to
2459c48
Compare
| ) { | ||
| super(ast, collected); | ||
|
|
||
| if (process.env.NODE_ENV === 'test') { |
There was a problem hiding this comment.
Ugh, this is very painful. I'd rather lose the ability of measuring coverage than do this tbh.
There was a problem hiding this comment.
it's not in the standalone code but it's on the transpiled one. I am planning on an improvement to test this without having the test code in the source file.
There was a problem hiding this comment.
it became a whole mini project, but since the change is so big it just made sense that the proper testing would require that much care.
Janther
force-pushed
the
constructor-experiment
branch
2 times, most recently
from
01bfaf1 to
14bfc08
Compare
… the constructor from the Slang object
Janther
force-pushed
the
constructor-experiment
branch
from
a9cc2d3 to
1044131
Compare
| this.variant = | ||
| variant instanceof slangAst.Block | ||
| ? new Block(variant, collected, options) | ||
| : createNonterminalVariant(variant, collected, options); |
There was a problem hiding this comment.
I'm curious why this is different. Why not have [slangAst.Block, Block] in the array above?
There was a problem hiding this comment.
because the creation of the createNonterminalVariant happens at the file level so is the first thing being executed, Block.ts depends on Statements.ts which depends on Statement.ts which calls Block at the very beginning.
it's different if it's inside a function but we want the call to createNonterminalVariant to be the first thing to happen when we load the file and then forget about it. I believe with having createNonterminalVariant declared in another file would solve this, but I would have to do it for each file to keep consistency just to solve that one case. which wouldn't save much file size compared to the amount of work put behind.
or maybe delay the execution of the function with an event 🤔 but that would made the correctness of the code depend on the js event loop.
Here we only have one exception which is that for that one case we do a normal check which seemed for me a good compromise.
2 participants
this one started as a though experiment based on the repetitive pattern of checking an instance in order to create the correct wrapping object.
👎 We lose some typechecks as this solution is more general.
👎 We lose strict coverage when there is a particular variant for which we don't have a test.(this was fixed in 47b9a4a)😐 The types for the constructors and the keys in the
Mapcould use some work.😐
YulStatementandStatementhad to check manually forYulBlockandBlockbecause it generated a cyclical dependency (I could have solved this by having the factory be called in a different file but that was a bit too much and the noise is not that big).👍 Built size decreased by 5KB 😮
Update:
Instead of a
forloop we can use aMapusing the Slang class constructor as key for aMapand use that tofindthe corresponding AST constructor. We would lose the proper inheritance tree check thatinstanceofdoes. (slight performance increase in exchange for theinstanceofmatch)Also if we choose to use the class name as the key, we can use an
objectinstead of aMapwhich would result in a performance improvement, but that would deviate from the currentinstanceofapproach.I'm open to feedback here. (slightly bigger performance increase in exchange for a much looser match)